Conversation
Fokko
left a comment
Fokko
left a comment
There was a problem hiding this comment.
Thanks for working on this @jayceslesar, sorry for the late review.
I think this is a great start, I left some comments, let me know what you think!
smaheshwar-pltr
left a comment
smaheshwar-pltr
left a comment
There was a problem hiding this comment.
Thanks for the PR @jayceslesar, using InpsectTable to get orphaned files to submit to the executor pool is a nice idea! Just some concerns / suggestions / debugging help 😄
kevinjqliu
left a comment
kevinjqliu
left a comment
There was a problem hiding this comment.
Thanks for the PR! I added a few comments. ptal :)
|
a meta question, wydt of moving the orphan file function to its own file/namespace, similar to how to use i like the idea of having all the table maintenance functions together, similar to delta table's optimize |
I think that makes sense -- would #1880 end up there too? Also ideally there is a CLI that exposes all the maintenance actions too right? I think moving things to a new |
That's a good point. However, I think we should be able to either run them separate as well. For example, delete orphan files won't affect the speed of the table, so it is more of a maintenance feature to reduce object storage costs. Delete orphan files can also be pretty costly because of the list operation, ideally you would delegate this to the catalog that uses, for example, s3 inventory. |
|
@Fokko we probably also want pyiceberg to have some idea about https://iceberg.apache.org/spec/#delete-formats right? Is it currently aware of those files? |
|
@jayceslesar I believe the merge-on-read delete files (positional deletes, equality deletes, and deletion vectors) are returned by the all-files. The only part that's missing is the partition statistics files. |
Sounds good, I will add the partition statistics files when that is merged! |
|
Once issue I've found with this PR is that the catalog properties need to propagate to |
pyiceberg/table/maintenance.py
Outdated
| flat_known_files: set[str] = reduce(set.union, all_known_files.values(), set()) | ||
|
|
||
| scheme, _, _ = PyArrowFileIO.parse_location(location) | ||
| pyarrow_io = PyArrowFileIO() |
There was a problem hiding this comment.
| pyarrow_io = PyArrowFileIO() | |
| pyarrow_io = PyArrowFileIO(properties=self.tbl.catalog.properties) |
There was a problem hiding this comment.
Id like to see if I can achieve this without pyarrow and will attempt to do so after working in #2146
pyiceberg/table/maintenance.py
Outdated
| if older_than is None: | ||
| older_than = timedelta(0) | ||
| as_of = datetime.now(timezone.utc) - older_than | ||
| all_files = [f.path for f in fs.get_file_info(selector) if f.type == FileType.File and f.mtime < as_of] |
There was a problem hiding this comment.
| all_files = [f.path for f in fs.get_file_info(selector) if f.type == FileType.File and f.mtime < as_of] | |
| all_files = [f"{scheme}://{f.path}" for f in fs.get_file_info(selector) if f.type == FileType.File and f.mtime < as_of] |
| try: | ||
| import pyarrow as pa # noqa: F401 | ||
| except ModuleNotFoundError as e: | ||
| raise ModuleNotFoundError( | ||
| "For deleting orphaned files with a PyArrowFileIO, PyArrow needs to be installed" | ||
| ) from e |
There was a problem hiding this comment.
will this error ever happen? If the table's io is a PyArrowFileIo I think we've already verified that PyArrow is installed
There was a problem hiding this comment.
We dont ask if its pyarrowfilio we ask if it isnt fsspecfilio
Co-authored-by: aammar5 <89264433+aammar5@users.noreply.github.com>
|
Going to get around adding tests for both types of FileIO... @Fokko @kevinjqliu anything else you think we need here? |
|
@jayceslesar how's this coming? Let me know if i can help with anything. Id like to use this in prod as well! |
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that's incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions. |
|
This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time. |
7 participants
Closes #1200
Rationale for this change
Ability to do more table maintenance from pyiceberg (iceberg-python?)
Are these changes tested?
Added a test!
Are there any user-facing changes?
Yes, this is a new method on the
Tableclass.